Skip to content

Fix whitespace tooling edge cases#777

Open
johnml1135 wants to merge 5 commits intomainfrom
fix-whitespace-tooling
Open

Fix whitespace tooling edge cases#777
johnml1135 wants to merge 5 commits intomainfrom
fix-whitespace-tooling

Conversation

@johnml1135
Copy link
Contributor

@johnml1135 johnml1135 commented Mar 20, 2026

Summary

  • replace the invalid Unicode warning output in the whitespace checker with ASCII-safe text
  • make the whitespace checker/fixer handoff explicit with exit codes and keep the results log as a parsing/inspection artifact
  • converge whitespace and commit-message CI helper scripts on PowerShell and remove the duplicate bash implementations
  • move commit-message summary formatting out of workflow YAML and into a dedicated Build/Agent helper
  • tighten the commit-message workflow so sticky PR comments follow the lint step outcome and do not warn on successful runs with no prior comment to hide

Why

The original whitespace fix addressed real edge cases, but the helper scripts had drifted into a split local-vs-CI model:

  • local tasks were already using PowerShell helpers
  • CI still used separate bash implementations
  • the whitespace wrapper inferred checker state from the presence of check-results.log
  • the commit-message workflow used the sticky PR comment action in a way that could emit a misleading Comment body cannot be blank warning on success
  • the workflow had accumulated a non-trivial inline PowerShell block for result formatting, which is harder to test and maintain than a normal helper script

This PR keeps the original whitespace fixes, makes the whitespace flow easier to reason about, removes duplicate bash maintenance, and clarifies the contract for both checks:

  • exit codes drive pass/fail and control flow
  • text logs are retained as human-readable payloads for summaries, parsing, and local inspection
  • workflow YAML orchestrates steps; reusable logic lives in Build/Agent

Landing place for the checks

Whitespace

  • CI workflow entry point: .github/workflows/check-whitespace.yml
  • Checker: Build/Agent/check-whitespace.ps1
  • Local wrapper/fixer: Build/Agent/check-and-fix-whitespace.ps1
  • Fixer: Build/Agent/fix-whitespace.ps1

Why it is split this way:

  • check-whitespace.ps1 owns detection and status reporting.
  • It returns explicit exit codes:
    • 0 = no whitespace issues
    • 2 = whitespace issues found
    • 1 = checker failed unexpectedly
  • It still writes check-results.log, but that file is now an artifact for parsing and inspection rather than the hidden control signal for the wrapper.
  • check-and-fix-whitespace.ps1 uses the exit code to decide whether it is safe to run the fixer.
  • fix-whitespace.ps1 uses check-results.log when available to target the reported files, and otherwise falls back to branch-introduced changes from merge-base(origin/main, HEAD)...HEAD.

Commit messages

  • CI workflow entry point: .github/workflows/CommitMessage.yml
  • Checker: Build/Agent/commit-messages.ps1
  • Summary/output helper: Build/Agent/publish-commit-message-summary.ps1

Why it is split this way:

  • commit-messages.ps1 owns the gitlint invocation and returns gitlint's exit code.
  • It also writes check_results.log so the same human-readable output can be reused in the job summary and sticky PR comment.
  • publish-commit-message-summary.ps1 owns result normalization and GitHub Actions output/summary formatting, so that logic is testable in PowerShell instead of being embedded in YAML.
  • The workflow now keys comment behavior off the actual lint step outcome instead of generic job success() / failure().
  • The sticky comment cleanup step uses only_update: true, so successful runs hide an existing sticky comment if one exists and do nothing otherwise.

Changes

Whitespace tooling

  • replace the invalid Unicode warning output with ASCII-safe text
  • stop using check-results.log as hidden control flow for the wrapper
  • use explicit checker exit codes to distinguish clean runs, whitespace findings, and unexpected failures
  • make check-results.log deterministic by recreating it at the start of the checker
  • keep the fallback file selection based on merge-base(origin/main, HEAD)...HEAD, so it targets branch-introduced changes rather than drift on the moving tip of main

Commit-message tooling

  • make the PowerShell helper the single implementation used by both local tasks and CI
  • preserve the CI summary and sticky PR comment behavior while moving the workflow steps to pwsh
  • install gitlint on demand when it is not already available
  • make check_results.log deterministic by recreating it at the start of the checker
  • move summary/output formatting into Build/Agent/publish-commit-message-summary.ps1
  • make the summary helper treat an empty successful result as No problems found.
  • tie sticky-comment post/hide behavior to the actual lint step outcome
  • remove the misleading Comment body cannot be blank warning on successful runs with no existing sticky comment

Cleanup

  • remove the duplicate bash helper scripts for whitespace and commit-message checks
  • reduce workflow YAML to orchestration rather than embedded formatting logic

Validation

  • ./Build/Agent/commit-messages.ps1
  • ./Build/Agent/check-whitespace.ps1
  • ./Build/Agent/check-and-fix-whitespace.ps1
  • direct smoke-test execution of ./Build/Agent/publish-commit-message-summary.ps1 for empty-success and populated-failure inputs
  • CI: Full local check
  • reran ./Build/Agent/commit-messages.ps1 after creating follow-up commits

This change is Reviewable


This change is Reviewable

Copilot AI review requested due to automatic review settings March 20, 2026 18:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the whitespace checking/fixing scripts against edge cases (invalid Unicode output, missing/empty results artifacts, and incorrect diff invocation) and adjusts the wrapper behavior to avoid running the fixer when the checker produced no usable output.

Changes:

  • Replaced Unicode warning output with ASCII-safe text in the whitespace checker.
  • Ensured check-results.log exists even when there are zero commits to report, and switched parsing to use the in-memory $log.
  • Adjusted diff invocation in the fixer and gated running the fixer based on checker outcome/artifacts.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
Build/Agent/fix-whitespace.ps1 Updates the fallback git diff call used to determine files to fix
Build/Agent/check-whitespace.ps1 Makes results logging resilient to empty output and replaces Unicode warning text
Build/Agent/check-and-fix-whitespace.ps1 Cleans stale results and gates fixer execution based on checker outcome

@github-actions
Copy link

github-actions bot commented Mar 20, 2026

NUnit Tests

    1 files  ±0      1 suites  ±0   5m 52s ⏱️ -19s
4 074 tests ±0  4 003 ✅ ±0  71 💤 ±0  0 ❌ ±0 
4 083 runs  ±0  4 012 ✅ ±0  71 💤 ±0  0 ❌ ±0 

Results for commit 256d564. ± Comparison against base commit 2565ac1.

♻️ This comment has been updated with latest results.

@johnml1135 johnml1135 force-pushed the fix-whitespace-tooling branch from 26d1fa8 to a5bbb76 Compare March 25, 2026 14:31
Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check this on a clean commit? (good commit message and no white space issues)

@jasonleenaylor reviewed 10 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on johnml1135).

@jasonleenaylor
Copy link
Contributor

.github/workflows/CommitMessage.yml line 24 at r3 (raw file):

      shell: pwsh
      run: >
        ./Build/Agent/publish-commit-message-summary.ps1

9 lines of bash replaced by 4 lines of powershell plus a 69 line powershell script for the same functionality doesn't feel like an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants